Skip to content

For the fields fetch phase, avoid reloading stored fields. #58196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 17, 2020
Merged

For the fields fetch phase, avoid reloading stored fields. #58196

merged 3 commits into from
Jun 17, 2020

Conversation

@jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Jun 16, 2020 •
edited
Loading

This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.

This improves the performance of fields loading so that it's very close to
source filtering. See #55363 (comment) for more details.

Relates to #55363.

@jtibshirani jtibshirani added :Search/Search Search-related issues that do not fall into other categories >refactoring labels Jun 16, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 16, 2020

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jun 16, 2020
@jtibshirani jtibshirani mentioned this pull request Jun 16, 2020
Closed
10 tasks
@jtibshirani
For the fields fetch phase, avoid reloading stored fields.
2c7279d 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
@jtibshirani jtibshirani requested a review from nik9000 June 16, 2020 20:22
@jtibshirani jtibshirani force-pushed the share-stored-fields branch from 47c46f9 to 2c7279d Compare June 16, 2020 20:22
@jtibshirani jtibshirani requested a review from jimczi June 16, 2020 20:23
jimczi
jimczi approved these changes Jun 16, 2020
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a question, lgtm otherwise

server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java
context.fetchSourceContext(new FetchSourceContext(true));
}
fieldsVisitor = new FieldsVisitor(context.sourceRequested());
boolean loadSource = context.sourceRequested() || context.fetchFieldsContext() != null;
Copy link
Contributor

@jimczi jimczi Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have the same logic in the else below or do we expect users to not use stored_fields in conjunction with the new fields option ?

Copy link
Member

@nik9000 nik9000 Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be safest to add it below too.

Copy link
Contributor Author

@jtibshirani jtibshirani Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thanks for catching this. I'll push an update.

nik9000
nik9000 approved these changes Jun 16, 2020
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

rest-api-spec/src/main/resources/rest-api-spec/test/search/330_fetch_fields.yml Outdated
keyword: "x"
integer: 42

- do:
Copy link
Member

@nik9000 nik9000 Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to stick refresh on the index request.

Copy link
Contributor Author

@jtibshirani jtibshirani Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is nicer, especially when there's only one index request.

@jtibshirani
Copy link
Contributor Author

jtibshirani commented Jun 17, 2020

@elasticmachine run elasticsearch-ci/2

@jtibshirani jtibshirani merged this pull request into elastic:field-retrieval Jun 17, 2020
@jtibshirani jtibshirani deleted the share-stored-fields branch June 17, 2020 17:36
jtibshirani added a commit that referenced this pull request Jun 17, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (#58196)
266683d 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jun 26, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (#58196)
2a15f6c 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jun 30, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (#58196)
476e454 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jun 30, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (#58196)
d50eea5 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 8, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (#58196)
b8fcdfe 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 14, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (#58196)
680d49c 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 15, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (#58196)
075bead 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 18, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (#58196)
d63f769 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 21, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (#58196)
be458e6 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit that referenced this pull request Jul 23, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (#58196)
37cbaa7 
This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Jul 27, 2020
@jtibshirani
For the fields fetch phase, avoid reloading stored fields. (elastic#5…
828f514 
…8196)

This PR updates FetchFieldsPhase to override hitExecute instead of hitsExecute
(plural). This way, we can make sure that the stored fields (including _source)
are only loaded once per hit as part of FetchPhase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nik9000 nik9000 nik9000 approved these changes

@jimczi jimczi jimczi approved these changes

Assignees

No one assigned

Labels

>refactoring :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jtibshirani @elasticmachine @nik9000 @jimczi